[#498] cleanup message type details #499
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #499 +/- ##
==========================================
+ Coverage 79.21% 79.26% +0.05%
==========================================
Files 200 200
Lines 23728 23762 +34
==========================================
+ Hits 18795 18835 +40
+ Misses 4933 4927 -6
|
| // TypeVariant::Dynamic == slice and only here it makes sense to loan more than one element | ||
| debug_assert!(slice_len == 1 || self.payload_type_variant() == TypeVariant::Dynamic); | ||
|
|
||
| self.loan_slice_uninit_impl(slice_len, self.payload_size * slice_len) |
There was a problem hiding this comment.
I have the feeling there is still not a clear definition what payload means and it is used in different contexts with a different meaning, resulting in confusion at some point.
No need to change it now but we need to have unambiguous terminology, else this will haunt us in regular intervals.
There was a problem hiding this comment.
Just out of my head:
- payload == all bytes that belong to the user produced data
- slice can be a form of payload
- a slice consists of 1..N elements, therefore we have in this context an element_size?
| /// | ||
| /// In this case the number of elements relates to the element defined in the | ||
| /// [`MessageTypeDetails`](crate::service::static_config::message_type_details::MessageTypeDetails). | ||
| /// When the element has a `payload.size == 40` and the `Sample::payload().len() == 120` it |
There was a problem hiding this comment.
This is what I mean, the term payload is used in multiple contexts and a comment is necessary to point out what it means. In this case it would make more sense to talk about element.size or item.size. A payload can consist of one or multiple elements/items and all elements make up the payload size.
There was a problem hiding this comment.
I agree, I just speak about payload.size since this is the member of MessageTypeDetails.
I will refactor it so that it becomes more clear.
There was a problem hiding this comment.
I tried refactoring this, but this will be a huge task and there are a lot of files affected. I think it would be best to have a separate session for this.
A payload can consist of one or multiple elements/items and all elements make up the payload size.
This is only true for the sliced API. Otherwise, the element is the payload, and it will lead to confusion when only working with this API when suddenly the there is also an element.size. One may think then it relates to members of a struct or something else.
There was a problem hiding this comment.
Btw. we are discussing here a comment that becomes only relevant for an internal API that the user shall never use.
I acknowledge that this is important and we have to refine the code. But making the code reflecting the terms correctly without increasing the code complexity is a challenge here in my opinion.
There was a problem hiding this comment.
The discussion is about spending some time now to reduce cognitive load for us and other maintainers later.
Although it is internal API, there is still an audience, so clarity still has its merits. In the absolute worst case, we slow down contributions or even scare away potential contributors due to the amount of inside information required.
Leave it up to you to decide if it is worth doing here.
There was a problem hiding this comment.
@elfenpiff Let's adjust this in a follow-up if required.
Notes for Reviewer
In this PR the API used when overriding the
MessageTypeDetailsis strictly separated from the end user API.MessageTypeDetailsyou must use[CustomPayloadMarker]as message typePublisher::loan_custom_payload()Subscriber::receive_custom_payload().The reason is, that in those cases the
SampleandSampleMuthandle the data a bit different. When callingPublisher::loan_custom_payload(123)and 123 corresponds to the number of elements, this number is stored in the samples header butSample::payload().len()will return123 * MessageTypeDetails::payload.size, the bytes used by the sample. When theMessageTypeDetails::payload.sizeis not overridden,Sample::payload().len() == Header::number_of_elements().The publish subscribe header contains now the
number_of_elementsthat are contained in the sample. The payload layout was removed. In the C API one can now easily obtain the underlying number of elements by just accessing this number directly from the header - no morepayload.len() / sizeof(Payload)stuff.Pre-Review Checklist for the PR Author
SPDX-License-Identifier: Apache-2.0 OR MITiox2-123-introduce-posix-ipc-example)[#123] Add posix ipc example)task-list-completed)Checklist for the PR Reviewer
Post-review Checklist for the PR Author
References
Closes #498